Replace _handle_request elif chains with a handler registry #65827#68106
Open
JH-A-Kim wants to merge 81 commits into
Open
Replace _handle_request elif chains with a handler registry #65827#68106JH-A-Kim wants to merge 81 commits into
JH-A-Kim wants to merge 81 commits into
Conversation
JH-A-Kim
force-pushed
the
refactor-handle_request-with-handler-registry
branch
2 times, most recently
from
June 5, 2026 22:18
e220072 to
849d275
Compare
Add the registry for request_handlers.py so we can refactor _handle_request in handlers
…NotImplementedError with registry dispach
JH-A-Kim
force-pushed
the
refactor-handle_request-with-handler-registry
branch
from
June 6, 2026 02:32
8d75303 to
4de5af1
Compare
… tests client is now a required field in WatchedSubProcess so we now have to pass a mock to satisfy the constructor
handle_mask_secret does not use the client field but it requires the client:Client because it needs to satisfy the uniform handler contract stipulated by the registry dispatcher
move GetAssetByName GetAssetByUri GetAssetsByAlias GetAssetEventByAsset GetAssetEventByAssetAlias TriggerDagRun GetDagRun GetDag GetTaskRescheduleStartDate GetTaskBreadcrumbs ValidateInletsAndOutlets CreateHITLDetailPayload GetTaskStore SetTaskStore DeleteTaskStore ClearTaskStore GetAssetStoreByName GetAssetStoreByUri SetAssetStoreByName SetAssetStoreByUri DeleteAssetStoreByName DeleteAssetStoreByUri ClearAssetStoreByName ClearAssetStoreByUri to decorator registry
remove GetAssetByName GetAssetByUri GetAssetsByAlias GetAssetEventByAsset GetAssetEventByAssetAlias TriggerDagRun GetDagRun GetDag GetTaskRescheduleStartDate GetTaskBreadcrumbs ValidateInletsAndOutlets CreateHITLDetailPayload GetTaskStore SetTaskStore DeleteTaskStore ClearTaskStore GetAssetStoreByName GetAssetStoreByUri SetAssetStoreByName SetAssetStoreByUri DeleteAssetStoreByName DeleteAssetStoreByUri ClearAssetStoreByName ClearAssetStoreByUri from isinstance chain
JH-A-Kim
marked this pull request as ready for review
June 7, 2026 04:09
JH-A-Kim
requested review from
amoghrajesh,
ashb,
dstandish,
ephraimbuddy,
hussein-awala,
jedcunningham and
kaxil
as code owners
June 7, 2026 04:09
JH-A-Kim
marked this pull request as draft
June 7, 2026 04:13
Replace cached_property with a property backed by _client. attrs was silently resolving the client to None at runtime, causing NoneType failures when trigger code requested connections or dag run state. The setter allows subclasses like TriggerRunnerSupervisor to inject or override the client during construction.
attrs was preventing a @Property named 'client' from being defined because it owned a field with the same name. Renaming the field to _client with alias='client' keeps the constructor interface unchanged while freeing up the 'client' name for the lazy-init property.
Member
|
@JH-A-Kim — converting this to draft because it currently has merge conflicts with See the PR quality criteria. This is an automated first-pass triage note drafted by an AI-assisted tool and may get things wrong; once you've addressed it, a real Apache Airflow maintainer takes the next look. (why the first pass is automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
potiuk
marked this pull request as draft
June 25, 2026 10:11
…andlers and supervisor tests
JH-A-Kim
marked this pull request as ready for review
June 26, 2026 03:11
Contributor
Author
|
@ashb Would it be possible to get a review? Thank you! |
Contributor
Author
|
@ashb Would i be able to get a review? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the changes specified in #65827
The goal of this pull request is to refactor the
_handle_requestmethod by replacing a long,isinstanceelifchain with a Handler Registry design pattern. This improves maintainability, and prevents future code bloat as new requests are added.In test_triggerer_job.py multiple tests failed on client as it was not seeing the
client. What ended up being the problem is thattriggerer_job_runner.pywas lazy initializing the client and was only creating a client when it strictly needed it, but becauseattrswas causing some conflicts with it, theclientwas being set toNonecausing failures. The solution ended up being removing the lazy initialization, creating the client inside thestartand then updating the tests cases to align with the new specification for the client being set inWatchedSubProcessinstead of the individual processesors.Key Changes
request_handlers.py. Shared handlers are now used through the decorator.raise NotImplementedError()with the registry system for WatchedSubProcess.super()for shared messages, and for any supervisor specific special cases they are still handled in-line.test_integration.py, andtest_triggerer_job.pytest cases updated to align with the new specification for client being set in WatchedSubProcess instead in individual processors.Handlers Moved
Moved the following handlers to the registry:
GetAssetByName
GetAssetByUri
GetAssetsByAlias
GetAssetEventByAsset
GetAssetEventByAssetAlias
TriggerDagRun
GetDagRun
GetDag
GetTaskRescheduleStartDate
GetTaskBreadcrumbs
ValidateInletsAndOutlets
CreateHITLDetailPayload
GetTaskStore
SetTaskStore
DeleteTaskStore
ClearTaskStore
Asset store CRUD:
GetAssetStoreByName
GetAssetStoreByUri
SetAssetStoreByName
SetAssetStoreByUri
DeleteAssetStoreByName
DeleteAssetStoreByUri
ClearAssetStoreByName
ClearAssetStoreByUri
Was generative AI tooling used to co-author this PR?
Generated-by: [Sonnet 4.6] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.Important
🛠️ Maintainer triage note for @JH-A-Kim · by
@potiuk· 2026-07-02 17:46 UTCSome review feedback from
@ferruzziis waiting on you:@ferruzzineed a reply or a fix.The ball is in your court — you've been assigned to this PR. Reply or push a fix in each thread, then mark them resolved.
Automated triage — may be imperfect; a maintainer takes the next look.